feat(database): add database user password update route #373
Conversation
curzolapierre
force-pushed
the
fix/cli/642/database_user_update
branch
2 times, most recently
from
7cacf91 to
6b3af9f
Compare
curzolapierre
force-pushed
the
fix/cli/642/database_user_update
branch
from
6b3af9f to
56333a3
Compare
databases.go
Outdated
| DatabaseUser DatabaseUpdateUserParam `json:"database_user"` | ||
| } | ||
|
|
||
| // DatabaseUpdateUser updates an user to the given database addon |
There was a problem hiding this comment.
typo: it's "a user" not "an user"
databases.go
Outdated
| } | ||
|
|
||
| // DatabaseUpdateUser updates an user to the given database addon | ||
| func (c *Client) DatabaseUpdateUser(ctx context.Context, app, addonID, username string, user DatabaseUpdateUserParam) (DatabaseUser, error) { |
There was a problem hiding this comment.
suggestion: the "user" variable is misleading in this case. It could be better to name it like "databaseUserParams" WDYT?
databases.go
Outdated
| } | ||
| err := c.DBAPI(app, addonID).SubresourceUpdate(ctx, "databases", addonID, "users", username, payload, &res) | ||
| if err != nil { | ||
| return res.DatabaseUser, errors.Wrapf(ctx, err, "create a user on database %s", addonID) |
There was a problem hiding this comment.
issue: it should be "update a user" right?
curzolapierre
force-pushed
the
fix/cli/642/database_user_update
branch
from
56333a3 to
c2015d2
Compare
There was a problem hiding this comment.
It's not directly related but can you fix the typo here:
"esource" --> "resource"
Line 27 in 07661a2
| payload := databaseUpdateUserPayload{ | ||
| DatabaseUser: databaseUserParams, | ||
| } | ||
| err := c.DBAPI(app, addonID).SubresourceUpdate(ctx, "databases", addonID, "users", username, payload, &res) |
There was a problem hiding this comment.
question: "databases" and "users" shouldn't be constants?
There was a problem hiding this comment.
I wonder, but IMO I prefer to see what is the route by seeing the string "databases".
But indeed it leaves room for potential manual errors.
However I think we should make this consistent with all other packages too.
It should be a dedicated issue I think.
There was a problem hiding this comment.
Yep, you are right it should be a dedicated issue.
Can you create one for that please 🙏
| payload := databaseUpdateUserPayload{ | ||
| DatabaseUser: databaseUserParams, | ||
| } | ||
| err := c.DBAPI(app, addonID).SubresourceUpdate(ctx, "databases", addonID, "users", username, payload, &res) |
There was a problem hiding this comment.
I wonder, but IMO I prefer to see what is the route by seeing the string "databases".
But indeed it leaves room for potential manual errors.
However I think we should make this consistent with all other packages too.
It should be a dedicated issue I think.
Related to Scalingo/cli#1034